Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spec changes for image extensions #298

Closed
wants to merge 24 commits into from

Conversation

natalieparellano
Copy link
Member

Supersedes #267

natalieparellano and others added 9 commits March 17, 2022 13:22
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
…le.d

Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Copy link
Member Author

natalieparellano commented Mar 17, 2022

This is still a bit rough... I'm going to add some comments to call out things that might be interesting for the reader. This branch includes commits from buildpack/0.8, which makes the diff harder to read, but the buildpack api changes more coherent. I'll rebase this branch once buildpack/0.8 is merged.

buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
During detection, an order definition MUST be resolved into individual buildpack implementations.
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations.

Order definitions for image extensions MUST NOT contain nested orders. If an order definition for image extensions is present, it will be pre-pended to the order definition for buildpacks (as if the order definition for image extensions were an order buildpack).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence (while "accurate", I think) doesn't make much sense... I am not sure how to improve it without re-working the entire section.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"During detection, order definitions MUST be resolved into individual buildpack implementations. If present, Image Extensions MAY provide an order definition. Buildpacks MUST provide an order definition."

Not sure if that covers the intent.. which itself kinda hints the original still has wiggle room.. (It's possible in my rephrasing to interpret that each extension would supply an order definition, or that there would be one for all extensions).

The original leaves bare the possibilty that an order defintiion for extensions is required, although the extensions themselves remain optional (as the binding of the 'if present' can be made to 'image extensions' rather than 'order definition').

buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Copy link
Member Author

This almost certainly needs more changes but I am marking it ready for review in the interest of getting feedback.

@natalieparellano natalieparellano marked this pull request as ready for review March 23, 2022 16:50
@natalieparellano natalieparellano requested a review from a team as a code owner March 23, 2022 16:50
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
- A directory containing application source code
- A shell, if needed,

For each buildpack in each group in order, the lifecycle MUST execute `/bin/detect`.
For each buildpack implementation in each group in order, the lifecycle MUST execute `/bin/detect`. Image extensions and buildpacks are both buildpack implementations. For simplicity, the following section will refer to both as "buildpacks". Image extensions MUST always be optional during detection.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we stop using the term "buildpack implementation"? Or at least swap the word order to "implementation buildpack" if we can't think of a better term for a non-meta-buildpack? Meta-buildpacks are still implementations of buildpacks, so I think it's difficult to understand these sections.

This could be a separate PR, but I feel like adding image extensions makes this even more confusing.

Some name ideas:

  • atomic buildpacks
  • executable buildpacks
  • concrete buildpacks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like concrete buildpacks.. has a nice construction theme to it ;p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #300 - I went with "executable buildpack" because that seemed most natural according to the definition I used.

@@ -373,12 +386,14 @@ A buildpack's mixin requirements must be satisfied by the stack in one of the fo

#### Order Resolution

During detection, an order definition MUST be resolved into individual buildpack implementations.
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations.
During detection, order definitions for image extensions and buildpacks MUST be resolved into a single order definition containing only atomic buildpacks.

During detection, an order definition MUST be resolved into individual buildpack implementations.
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations.

Order definitions for image extensions MUST NOT contain nested orders. If an order definition for image extensions is present, it will be pre-pended to the order definition for buildpacks (as if the order definition for image extensions were an order buildpack).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Order definitions for image extensions MUST NOT contain nested orders. If an order definition for image extensions is present, it will be pre-pended to the order definition for buildpacks (as if the order definition for image extensions were an order buildpack).
Order definitions for image extensions MUST NOT contain nested orders.
If an order definition for image extensions is present, it MUST be prepended to the order definition for buildpacks before the resolution process occurs.

To make this more clear, I think we should define the following six terms early in the spec:

  1. Order buildpack
  2. Atomic buildpack
  3. Order image extension (not currently allowed)
  4. Atomic image extension
  5. Order build module (currently only order buildpack)
  6. Atomic build module (atomic buildpacks and image extensions)

The names are just suggestions.
Maybe it's easier to say that image extensions are buildpacks?

  1. Normal order buildpack
  2. Normal atomic buildpack
  3. Image extension order bulidpack (not currently allowed)
  4. Image extension atomic buildpack
  5. Order buildpack (only normal buildpacks allowed)
  6. Atomic buildpack (atomic normal and extension buildpacks)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sclevine what is a build module? What is the difference between an order build module and an atomic build module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hash this out in https://github.com/buildpacks/spec/pull/300/files - we can rebase this PR when that one is merged.

- A directory containing application source code
- A shell, if needed,

For each buildpack in each group in order, the lifecycle MUST execute `/bin/detect`.
For each buildpack implementation in each group in order, the lifecycle MUST execute `/bin/detect`. Image extensions and buildpacks are both buildpack implementations. For simplicity, the following section will refer to both as "buildpacks". Image extensions MUST always be optional during detection.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why image extensions MUST be optional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sclevine could you elaborate?

image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
Correspondingly, each `/bin/build` executable:

- MAY read from the `<app>` directory.
- MUST NOT write to the `<app>` directory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How/where will we be enforcing this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same way we enforce buildpacks not writing to other buildpacks' layers directory (wishful thinking) :)

This is something that we could add in theory, but it would to subject to the same concerns outlined in buildpacks/rfcs#155

image-extension.md Outdated Show resolved Hide resolved
ARG base_image
FROM ${base_image}
```
- MUST NOT contain any other `FROM` instructions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking to discussion here: https://github.com/buildpacks/rfcs/pull/173/files#r794629405

The concern is that we could end up switching to a base image that doesn't have buildpacks on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather have this as a 'SHOULD' than a MUST.. it's doable, IF you really know what you are doing, and closing that door with a MUST might end up closing an escape hatch that someone might need =)

I've tried swapping build images in the PoC (and copying the /cnb folder from the previous image via multistage) and it is doable, but I hit odd issues executing the build step after the extension step, that I suspect were related to that switch.. I suspect with enough care, with certain constraints it could be done.. eg, if the image switched to was built as a stack image that includes the buildpacks content, etc, etc, etc.

Comment on lines +153 to +159
When extending the run image, a Dockerfile MAY indicate that it preserves the ability to rebase by including the following instruction:

```bash
LABEL io.buildpacks.rebasable=true
```

For the final image to be rebasable, the provided run image and all applied Dockerfiles MUST indicate rebasability.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen in case this label is set to true? What are the cases when an extension ran and the output image is still rebaseable?

One other thing I am unsure about is how we will uniquely identify a "family" of base images that can be rebased with one another.

I know we thought about replacing stack id with some other identifier that indicates the "family" of base images - how does that fit in here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole rebase picture with extensions is still unclear to me. It would be nice if we can have some actual examples of cases where/how this can work with platforms like pack or kpack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking / hoping that an extension can still set rebaseable=true, where it modifies the build image (say to install runtimes, tools to build etc) .. but limits the runtime changes to just using 'FROM' to switch to a different run image.. I think this would still qualify to be rebaseable, as future run images can still be swapped in.. provided they still contain the required dependencies that were met by the substituted run image. In theory this allows for the existence of a generic builder image that tailors itself to install runtimes/tools etc, working in conjunction with a set of run images to cover the most common scenarios.. and maybe, also in conjunction with a generic (non rebaseable) run image to cover the edge cases where the common image was insufficient.

Copy link
Member Author

@natalieparellano natalieparellano Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, in the case where a Dockerfile is applied to the run image, but all it does is switch the run image (using a FROM instruction):

  • Should we publish a new "extended" run image (the <target-image> argument provided to the extender)
    • Opinion: no, that seems confusing
  • Should we run genpkgs?

image-extension.md Outdated Show resolved Hide resolved
platform.md Show resolved Hide resolved
natalieparellano and others added 2 commits March 24, 2022 11:59
Signed-off-by: Natalie Arellano <narellano@vmware.com>

Co-authored-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@hone hone marked this pull request as draft April 6, 2022 18:04
@natalieparellano
Copy link
Member Author

natalieparellano commented Apr 14, 2022

Just to provide an update on this - I see this effort and the "run image SBOM" effort as sort of orthogonal at this point. The diagram below depicts what the path to implementation might look like:

Screen Shot 2022-04-14 at 9 54 03 AM

We had discussed a phased implementation for Dockerfiles previously and this seems to be prudent given that we're still working out some issues with kaniko. As a next step, I could update this PR (or open a new one) to capture only the changes needed for the first phase (the green box in the diagram above).

@natalieparellano
Copy link
Member Author

To expand on the conversation from 4/14 Working Group, the following (simpler) diagrams describe the extend / sbom flows for two phases of the implementation. cc @samj1912 @sclevine

First phase:

Screen Shot 2022-04-14 at 11 37 04 AM

Final phase:

Screen Shot 2022-04-14 at 11 39 06 AM
Screen Shot 2022-04-14 at 11 39 16 AM

@natalieparellano
Copy link
Member Author

natalieparellano commented Apr 14, 2022

After further discussion in 4/14 Office Hours, it seems that the simplest and best path forward for dealing with run image SBOMs is to make this the responsibility of the signer binary.

After the app image is exported, the signer will attempt to locate the run image SBOM on the registry; if an SBOM is not found, the signer will pull the run image in order to scan it - this will negatively impact performance, but can be avoided by:

  • ensuring the run image has an SBOM in the first place (if you do care about SBOMs)
  • skipping this step entirely (if you don't care about SBOMs)

This will also simplify the lifecycle implementation. We can drop the notion of run image SBOM and genpkgs from this effort as buildpacks/rfcs#195 will now capture these concerns.

@natalieparellano
Copy link
Member Author

Closing in favor for #307 and #308. For posterity, here is a revised implementation plan following some discussion (and removing the run image SBOM dependency):

Screen Shot 2022-04-28 at 11 34 05 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants